Skip to content

Conversation

@JensenWei007
Copy link
Contributor

  • Add necessary platform driver support
  • Modify some startup processes and assert
  • Fixed some issues

@JensenWei007 JensenWei007 force-pushed the rv branch 2 times, most recently from 207fe36 to c1aa3d9 Compare September 16, 2025 05:45
@fslongjin
Copy link
Member

bugbot run

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is being reviewed by Cursor Bugbot

Details

You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.

To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.

Bug: Kernel Stack Initialization Mismatch

The init_idle function assumes processor 0 is the Boot Strap Processor (BSP) for kernel stack initialization. This conflicts with the removed assertion and the TODO comment noting that the boot hart might not be 0 on platforms like riscv-vf2. This mismatch can lead to incorrect kernel stack setup for the actual BSP and processor 0, potentially causing system instability.

kernel/src/process/idle.rs#L38-L46

for i in 0..PerCpu::MAX_CPU_NUM {
let kstack = if unlikely(i == 0) {
let stack_ptr =
VirtAddr::new(Self::stack_ptr().data() & (!(KernelStack::ALIGN - 1)));
// 初始化bsp的idle进程
let mut ks = unsafe { KernelStack::from_existed(stack_ptr) }
.expect("Failed to create kernel stack struct for BSP.");
unsafe { ks.clear_pcb(true) };
ks

Fix in Cursor Fix in Web


if cmd.data_expected() {
let buffer = // TODO: dirty
buffer.unwrap_or(unsafe { core::slice::from_raw_parts_mut(core::ptr::NonNull::dangling().as_ptr(), 64) });
assert!(buffer_offset == 0);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Invalid Memory Access in send_cmd Function

The send_cmd function creates a mutable slice from a dangling pointer when the buffer argument is None. This results in a 64-element slice pointing to invalid memory, which is then accessed during data transfers. This leads to undefined behavior, potential memory corruption, and system instability. This also bypasses the debug_assert on line 404 when data is expected.

Fix in Cursor Fix in Web

assert!(buf.len() == BLOCK_SIZE);

#[allow(mutable_transmutes)]
let buf = unsafe { core::mem::transmute(buf) };
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Unsafe Transmute Violates Rust Memory Safety

The write_block function uses mem::transmute to convert an immutable &[u8] slice into a mutable &mut [usize] slice. This violates Rust's aliasing rules and memory safety, leading to undefined behavior. The transmute also changes the element type, which can cause issues with data interpretation and alignment.

Fix in Cursor Fix in Web

impl BUFADDRU {
pub fn offset() -> usize {
0xA0
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Incorrect Offset for 64-bit Buffer Address Upper Part

The BUFADDRL and BUFADDRU structs incorrectly share the same offset (0xA0). As these represent the lower and upper 32-bit parts of a 64-bit buffer address, BUFADDRU needs a distinct offset, likely 0xA4, consistent with other 64-bit address register pairs.

Fix in Cursor Fix in Web

let base = self.virt_base_address() as *mut CTRL;
let ctrl = self.control_reg().with_fifo_reset(true);
unsafe { base.byte_add(*self.fifo_offset.get()).write_volatile(ctrl) }
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: FIFO Reset Error

In reset_fifo, the control register value is written to the FIFO data offset (0x600) instead of the correct control register offset (0x00). This prevents the FIFO from resetting and may corrupt other hardware registers.

Fix in Cursor Fix in Web

@Copilot Copilot AI review requested due to automatic review settings October 22, 2025 05:36
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds support for the VisionFive 2 (VF2) RISC-V platform by introducing platform-specific drivers and modifying startup procedures to accommodate hardware differences.

Key changes:

  • Implements VF2 SD/MMC card driver (DW MSHC controller)
  • Adjusts boot hart initialization logic for RISC-V platforms
  • Relocates architecture setup in the boot sequence for RISC-V

Reviewed Changes

Copilot reviewed 15 out of 16 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
kernel/src/process/idle.rs Temporarily disables boot hart assertion that assumes hart 0
kernel/src/mm/init.rs Moves RISC-V setup_arch() call earlier in memory initialization
kernel/src/init/initial_kthread.rs Adds VF2-specific init binary to search path
kernel/src/init/init.rs Guards setup_arch() call to prevent duplicate execution on RISC-V
kernel/src/filesystem/vfs/vcore.rs Adds /dev/sdio1 to root filesystem device candidates
kernel/src/driver/base/device/device_number.rs Adds MMC block device major number constant
kernel/src/arch/riscv64/pci/mod.rs Disables PCI initialization for VF2 platform
kernel/src/arch/riscv64/driver/vf2/mod.rs Implements VF2 MMC/SDIO probe and initialization
kernel/src/arch/riscv64/driver/vf2/dw_mshc/registers.rs Defines register structures for DW MSHC controller
kernel/src/arch/riscv64/driver/vf2/dw_mshc/mod.rs Module structure for DW MSHC driver components
kernel/src/arch/riscv64/driver/vf2/dw_mshc/mmc.rs Core MMC driver implementation with block device interface
kernel/src/arch/riscv64/driver/vf2/dw_mshc/dma.rs DMA descriptor structures for MSHC controller
kernel/src/arch/riscv64/driver/sbi.rs Modifies console output to handle line endings for RustSBI
kernel/src/arch/riscv64/driver/mod.rs Exports VF2 driver module
kernel/Cargo.toml Adds byte-slice-cast dependency for driver

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +30 to +31
// TODO: 这里需要修正一下判断逻辑
// 在 riscv-vf2 中, boot hart 不一定是0, 他可能是1, 因此会过不了
Copy link

Copilot AI Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The TODO comment should be in English to maintain consistency with the codebase. Additionally, it should reference the specific issue or provide more context about when this will be addressed.

Suggested change
// TODO: 这里需要修正一下判断逻辑
// riscv-vf2 中, boot hart 不一定是0, 他可能是1, 因此会过不了
// TODO: The logic here needs to be fixed. See issue #123 for details.
// In riscv-vf2, the boot hart is not always 0; it can be 1, so this check may fail.

Copilot uses AI. Check for mistakes.

("/bin/busybox", Some("init")),
("/bin/init", None),
("/bin/sh", None),
("/bin/riscv_rust_init", None), // 对vf2, 目前没做cmdline的适配, 加个默认寻找
Copy link

Copilot AI Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The inline comment should be in English to match the project's language standards.

Suggested change
("/bin/riscv_rust_init", None), // 对vf2, 目前没做cmdline的适配, 加个默认寻找
("/bin/riscv_rust_init", None), // For vf2, cmdline adaptation is not yet implemented, so add a default search

Copilot uses AI. Check for mistakes.

let fdt = open_firmware_fdt_driver().fdt_ref()?;
// vf2 不需要, 事实上 qemu 也不使用 pci,设备都是使用 mmio
// 因此其实初始化这个没有太大的意义, 先注释掉
// TODO: 如果取消注释且启用 vf2 平台, 那么需要补充 vf2 的 pcie 驱动
Copy link

Copilot AI Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These comments should be written in English to maintain consistency with the rest of the codebase.

Suggested change
// TODO: 如果取消注释且启用 vf2 平台, 那么需要补充 vf2 的 pcie 驱动
// TODO: If you uncomment this and enable the vf2 platform, you need to add the vf2 PCIe driver.

Copilot uses AI. Check for mistakes.

for c in s {
#[allow(deprecated)]
sbi_rt::legacy::console_putchar(*c as usize);
// 原本的是适配opensbi的, 但是对rustsbi的适配存在问题, 不能正确的解析'\r'
Copy link

Copilot AI Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment should be written in English to align with codebase documentation standards.

Suggested change
// 原本的是适配opensbi的, 但是对rustsbi的适配存在问题, 不能正确的解析'\r'
// The original implementation was adapted for OpenSBI, but there are compatibility issues with RustSBI, which cannot correctly parse '\r'

Copilot uses AI. Check for mistakes.

}

fn disk_range(&self) -> GeneralBlockRange {
// TODO: 实现自动读,下面的数字为fdisk -l DragonOS/bin/disk....img的结果
Copy link

Copilot AI Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This TODO comment should be in English to maintain documentation consistency.

Suggested change
// TODO: 实现自动读,下面的数字为fdisk -l DragonOS/bin/disk....img的结果
// TODO: Implement automatic reading. The number below is the result of running 'fdisk -l DragonOS/bin/disk....img'.

Copilot uses AI. Check for mistakes.

#[bits(16)]
pub oemid: usize, // OEM ID
#[bits(8)]
pub manfid: usize, // Manufacturer ID`
Copy link

Copilot AI Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Corrected backtick at the end of the comment. Should be a regular comment without the trailing backtick.

Suggested change
pub manfid: usize, // Manufacturer ID`
pub manfid: usize, // Manufacturer ID

Copilot uses AI. Check for mistakes.

}

fn set_parent(&self, _parent: Weak<LockedDevFSInode>) {
panic!("DeviceINode for MMC is not supportted!")
Copy link

Copilot AI Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Corrected spelling of 'supportted' to 'supported'.

Suggested change
panic!("DeviceINode for MMC is not supportted!")
panic!("DeviceINode for MMC is not supported!")

Copilot uses AI. Check for mistakes.

pub struct DES0 {
_reserved0: bool,
disable_interrupt_on_completion: bool,
last_decriptor: bool,
Copy link

Copilot AI Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Corrected spelling of 'decriptor' to 'descriptor'.

Suggested change
last_decriptor: bool,
last_descriptor: bool,

Copilot uses AI. Check for mistakes.

- Add necessary platform driver support
- Modify some startup processes and assert
- Fixed some issues

Signed-off-by: JensenWei007 <[email protected]>
@Samuka007
Copy link
Member

bugbot run

@cursor
Copy link

cursor bot commented Oct 22, 2025

Skipping Bugbot: Unable to authenticate your request. Please make sure Bugbot is properly installed and configured for this repository.

Comment on lines +72 to +75

#[cfg(target_arch = "riscv64")]
crate::arch::init::setup_arch().expect("setup_arch failed");

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里似乎x86的初始化也可以迁到这个位置?不需要分开编译条件处理? @fslongjin 怎么看
从原来的 kernel/src/init/init.rs:87 setup_arch()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants